Publish to PyPI #3
Conversation
as it is unmaintained
📝 WalkthroughWalkthroughAdds modern packaging via pyproject.toml (hatchling + uv-dynamic-versioning) and a package name/versioning change to deepnote-sqlalchemy-redshift, removes legacy setup.py and the universal wheel flag in setup.cfg, and adds a GitHub Actions "Publish to PyPI" workflow triggered on semver tags that builds, smoke-tests, and publishes. Replaces pkg_resources with importlib.metadata and packaging, and pkg_resources resource lookups with importlib.resources. Removes psycopg2cffi support and the redshift_sqlalchemy compatibility shim, updates dialects and tests to drop cffi variants, centralizes column-introspection into a private helper, adds import_dbapi/dbapi compatibility wrappers, and updates tests, docs config, dependency ranges, .gitignore, and tox configuration. Sequence Diagram(s)mermaid mermaid Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.14.0)sqlalchemy_redshift/__init__.py9-9: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/test_copy_command.py (1)
188-198: Nit: upper() redundantformat_type.value is already uppercase; upper() is unnecessary noise.
- """ % (creds, format_type.value.upper()) + """ % (creds, format_type.value)sqlalchemy_redshift/dialect.py (2)
723-735: Pass a mapping for enums, not a list__get_column_info expects a mapping for enums (checked via membership and subscripting). Redshift may not use enums, but passing {} is safer than [].
- enums=[], schema=col.schema, encode=col.encode, + enums={}, schema=col.schema, encode=col.encode,
1510-1526: redshift_connector import: chain ImportError and type annotateMinor polish.
- def import_dbapi(cls): + def import_dbapi(cls) -> "ModuleType": try: driver_module = importlib.import_module(cls.driver) @@ - except ImportError: - raise ImportError( - 'No module named redshift_connector. Please install ' - 'redshift_connector to use this sqlalchemy dialect.' - ) + except ImportError as err: + raise ImportError( + "No module named redshift_connector. Please install " + "redshift_connector to use this sqlalchemy dialect." + ) from NoneRemember to add:
from types import ModuleTypenear the top.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)README.rst(0 hunks)docs/conf.py(1 hunks)pyproject.toml(1 hunks)redshift_sqlalchemy/__init__.py(0 hunks)requirements-docs.txt(1 hunks)setup.cfg(0 hunks)setup.py(0 hunks)sqlalchemy_redshift/__init__.py(1 hunks)sqlalchemy_redshift/commands.py(3 hunks)sqlalchemy_redshift/ddl.py(1 hunks)sqlalchemy_redshift/dialect.py(8 hunks)tests/conftest.py(3 hunks)tests/rs_sqla_test_utils/models.py(1 hunks)tests/test_column_loading.py(2 hunks)tests/test_compiler.py(1 hunks)tests/test_copy_command.py(4 hunks)tests/test_default_ssl.py(1 hunks)tests/test_delete_stmt.py(4 hunks)tests/test_dialects.py(1 hunks)tests/test_materialized_views.py(4 hunks)tests/test_reflection.py(1 hunks)tests/test_reflection_views.py(2 hunks)tests/test_unload_from_select.py(12 hunks)tox.ini(1 hunks)
💤 Files with no reviewable changes (4)
- setup.py
- redshift_sqlalchemy/init.py
- README.rst
- setup.cfg
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_copy_command.py (1)
sqlalchemy_redshift/commands.py (2)
Format(438-444)Compression(447-450)
tests/test_default_ssl.py (1)
sqlalchemy_redshift/dialect.py (1)
Psycopg2RedshiftDialectMixin(1385-1426)
tests/test_column_loading.py (1)
sqlalchemy_redshift/dialect.py (1)
RedshiftDialect_psycopg2(1429-1445)
🪛 Ruff (0.14.0)
tests/conftest.py
235-235: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
sqlalchemy_redshift/commands.py
32-32: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
34-34: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
sqlalchemy_redshift/__init__.py
7-7: Avoid specifying long messages outside the exception class
(TRY003)
sqlalchemy_redshift/dialect.py
986-986: Missing return type annotation for private function __get_column_info
(ANN202)
999-999: Missing return type annotation for private function _handle_array_type
(ANN202)
1113-1114: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1119-1119: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1127-1129: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
1149-1149: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1155-1162: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
1415-1415: Missing return type annotation for classmethod import_dbapi
(ANN206)
1419-1421: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
1420-1420: Use f-string instead of format call
Convert to f-string
(UP032)
1424-1424: Missing return type annotation for classmethod dbapi
(ANN206)
1435-1435: Missing return type annotation for classmethod import_dbapi
(ANN206)
1437-1437: Use super() instead of super(__class__, self)
Remove super() parameters
(UP008)
1440-1440: Missing return type annotation for classmethod dbapi
(ANN206)
1442-1442: Use super() instead of super(__class__, self)
Remove super() parameters
(UP008)
1510-1510: Missing return type annotation for classmethod import_dbapi
(ANN206)
1528-1528: Missing return type annotation for classmethod dbapi
(ANN206)
🔇 Additional comments (42)
sqlalchemy_redshift/commands.py (1)
504-504: Docstring consistency updates look good.Minor wording adjustments to parameter documentation are consistent with the rest of the file.
Also applies to: 933-933
docs/conf.py (1)
76-76: LGTM!Explicit language setting is correct for Sphinx i18n workflows.
.gitignore (1)
62-65: LGTM!Standard uv-related ignores, consistent with the migration to uv tooling in pyproject.toml.
tests/rs_sqla_test_utils/models.py (1)
6-12: LGTM!Correct SA 1.4/2.0 compatibility pattern. The try/except fallback handles the declarative_base import location change.
tox.ini (4)
2-14: LGTM!Updated tox config with broader Python version coverage (3.9-3.13) and both SQLAlchemy 1.4/2.0 test matrices. Removal of psycopg2cffi is consistent with PR objectives.
17-28: LGTM!Test commands updated to use psycopg2 and redshift_connector only. Dependency version ranges include appropriate upper bounds for reproducibility.
30-35: LGTM!Lint environment updated with flake8 7.x and psycopg2-binary.
37-43: LGTM!Docs environment now explicitly uses Python 3.12.
pyproject.toml (3)
1-11: LGTM!Modern packaging config with hatchling and dynamic versioning. Python 3.9-3.13 support is appropriate.
39-42: LGTM!Three dialect entry points registered correctly, psycopg2cffi removed as intended.
44-64: LGTM!uv-dynamic-versioning and hatch build targets configured correctly. The wheel force-include ensures the CA bundle is packaged.
sqlalchemy_redshift/__init__.py (4)
1-2: LGTM!Correct migration from deprecated pkg_resources to importlib.metadata and packaging.version.
4-10: LGTM!psycopg2cffi removed from version check loop as intended. The exception message on line 7 is flagged by ruff (TRY003) but is appropriately informative.
12-12: LGTM!
__version__now uses importlib.metadata.version instead of pkg_resources.
14-28: LGTM!Registry registrations updated: psycopg2cffi dialect removed, three active dialects remain (redshift, redshift.psycopg2, redshift+redshift_connector). Consistent with pyproject.toml entry points.
.github/workflows/release.yml (1)
1-37: Verify PyPI trusted publisher configuration.The workflow uses OIDC trusted publishing (id-token: write) with
uv publish. Confirm the PyPI project accepts GitHub Actions OIDC from this repository by visiting the project's publishing settings on PyPI.requirements-docs.txt (1)
2-6: Review comment is based on outdated observations.The current requirements-docs.txt already has sphinx>=7.0.0, numpydoc>=1.5.0, jinja2>=3.1.0, psycopg2-binary>=2.9.9, and setuptools>=65.0.0, and git history confirms these versions were locked in place before this PR. The claimed version jumps (Sphinx 1.6.3→7.0.0, numpydoc 0.6.0→1.5.0) occurred in a prior commit, not in the current changes. No docs build verification is needed for this PR.
Likely an incorrect or invalid review comment.
tests/test_reflection.py (1)
229-229: LGTM!Standard execution options setup for AUTOCOMMIT mode.
tests/test_reflection_views.py (2)
29-29: Cosmetic spacing change.Functionally identical to previous code.
56-56: Cosmetic spacing change.Functionally identical to previous code.
tests/test_delete_stmt.py (4)
178-178: Modern SQLAlchemy select() syntax.Correct for v1.4+ and v2 compatibility.
205-205: Modern SQLAlchemy select() syntax.Correct for v1.4+ and v2 compatibility.
225-225: Modern SQLAlchemy select() syntax.Correct for v1.4+ and v2 compatibility.
269-269: Modern SQLAlchemy select() syntax.Correct for v1.4+ and v2 compatibility.
tests/test_compiler.py (1)
6-6: Modern SQLAlchemy select() syntax.Correct for v1.4+ and v2 compatibility.
tests/test_dialects.py (1)
3-6: Removed unmaintained psycopg2cffi dialect.Aligns with PR objectives. Tests now cover psycopg2 and redshift_connector only.
tests/test_unload_from_select.py (1)
33-33: Modern SQLAlchemy select() syntax throughout.All instances correctly updated for v1.4+ and v2 compatibility.
Also applies to: 60-60, 84-84, 110-110, 124-124, 145-145, 164-164, 208-208, 252-252, 282-282, 298-298, 327-327
tests/test_column_loading.py (2)
8-8: Removed unmaintained psycopg2cffi dialect.Aligns with PR objectives.
20-20: Test updated for single dialect.Correctly tests only RedshiftDialect_psycopg2 now.
sqlalchemy_redshift/ddl.py (1)
119-119: Documentation updated for modern SQLAlchemy syntax.Example now shows correct v2.0 select() usage.
tests/conftest.py (2)
66-70: Docstring clarityClearer ARN env-var note. No issues.
179-201: Consistent AUTOCOMMIT for create/drop DBUsing execution_options(isolation_level="AUTOCOMMIT") is the right approach across SA 1.4/2.0. LGTM.
tests/test_default_ssl.py (2)
8-10: Resource path may break under zip/PEXstr(files(...).joinpath(...)) isn’t guaranteed to be a real filesystem path in zip-based runtimes. If you target such environments, switch the runtime code to as_file(...) and manage lifetime; otherwise, verify the wheel installs the cert as a real file.
Do you intend to support zipapp/PEX? If yes, I can propose a safe as_file-based implementation in the dialect.
2-2: No action requiredThe project already enforces
requires-python = ">=3.9,<3.14", which satisfies the Python version requirement forimportlib.resources.files(). No fallback is needed since Python 3.8 is not supported. The certificate file is properly included in build configuration.tests/test_copy_command.py (4)
173-183: Switch to enums for formatGood move to typed enums; keeps API strict.
244-255: Enum-based compressionLooks correct and matches expected SQL.
284-295: BZIP2 enum usageConsistent with new enum API. LGTM.
311-321: JSON + GZIP enumsEnum usage keeps inputs validated; assertions align. LGTM.
tests/test_materialized_views.py (4)
14-15: Modern select() usageUsing select(table.c.id, ...) is 1.4/2.0-friendly. Good.
66-74: Key args accept string or columnBoth "id" and selectable.selected_columns.id cases compile identically. Good coverage.
82-90: Sortkey test paritySame pattern as distkey; fine.
100-108: Interleaved sortkey test parityConsistent and correct.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.toml(1 hunks)sqlalchemy_redshift/commands.py(3 hunks)sqlalchemy_redshift/dialect.py(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-19T17:46:02.517Z
Learnt from: m1so
PR: deepnote/sqlalchemy-redshift#3
File: sqlalchemy_redshift/dialect.py:1112-1121
Timestamp: 2025-10-19T17:46:02.517Z
Learning: When code is explicitly marked as vendored or copied from another project (e.g., "Copied from SQLAlchemy"), avoid suggesting style or formatting changes to maintain easier diff tracking with the upstream source.
Applied to files:
sqlalchemy_redshift/dialect.py
🪛 Ruff (0.14.0)
sqlalchemy_redshift/dialect.py
986-986: Missing return type annotation for private function __get_column_info
(ANN202)
999-999: Missing return type annotation for private function _handle_array_type
(ANN202)
1113-1114: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1119-1119: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1127-1129: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
1149-1149: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
1155-1162: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
1415-1415: Missing return type annotation for classmethod import_dbapi
(ANN206)
1420-1420: Use f-string instead of format call
Convert to f-string
(UP032)
1424-1424: Missing return type annotation for classmethod dbapi
(ANN206)
1435-1435: Missing return type annotation for classmethod import_dbapi
(ANN206)
1440-1440: Missing return type annotation for classmethod dbapi
(ANN206)
1510-1510: Missing return type annotation for classmethod import_dbapi
(ANN206)
1528-1528: Missing return type annotation for classmethod dbapi
(ANN206)
🔇 Additional comments (8)
pyproject.toml (2)
32-32: SQLAlchemy version constraint fixed.Aligns with tested versions (SA 1.4.15+ and 2.0.36+) per tox.ini. Addresses previous major concern.
38-41: Entry points correctly reflect psycopg2cffi removal.Per PR objective to remove psycopg2cffi, only psycopg2 and redshift_connector dialects remain—as expected.
sqlalchemy_redshift/commands.py (1)
32-34: Past review addressed; raw strings are appropriate.The
# noqadirectives have been removed, resolving the RUF100 linting warnings. Raw string literals are idiomatic for regex patterns and eliminate escape-sequence concerns.sqlalchemy_redshift/dialect.py (5)
6-6: LGTM: Import updates support new functionality.The addition of
files()from importlib.resources modernizes resource access, and the SQLAlchemy imports support the vendored column info method.Also applies to: 11-12, 24-24
984-1168: Vendored code from SQLAlchemy - no changes suggested.This method is explicitly copied from SQLAlchemy 1.4 for backward compatibility. Skipping style/formatting suggestions to maintain easier diff tracking with upstream.
Based on learnings
1415-1426: LGTM: SQLAlchemy 1.4/2.0 compatibility layer.The dual method pattern (
import_dbapi()for 2.0,dbapi()for <2.0) and delegation viasuper()are correct. Thefrom Noneon line 1421 properly suppresses exception context.Optional nitpick: Add return type annotations (
-> ModuleType) if desired, but not essential.Also applies to: 1434-1442
1510-1530: LGTM: Redshift connector compatibility layer.The implementation mirrors the psycopg2 pattern and includes appropriate version-specific logic for the connector driver.
75-75: Removal verified. psycopg2cffi is completely gone.The class definition was removed,
__all__entry is updated, and no remaining references exist. Change is correct.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sqlalchemy_redshift/__init__.py (1)
25-28: Fix dialect name inconsistency.Line 26 registers
"redshift+redshift_connector"butpyproject.tomlline 42 declares"redshift.redshift_connector". This mismatch will cause the entry-point to fail.Apply this diff:
registry.register( - "redshift+redshift_connector", "sqlalchemy_redshift.dialect", + "redshift.redshift_connector", "sqlalchemy_redshift.dialect", "RedshiftDialect_redshift_connector" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.toml(1 hunks)sqlalchemy_redshift/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
sqlalchemy_redshift/__init__.py
7-7: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
pyproject.toml (3)
1-3: LGTM: Modern build system.Standard hatchling + uv-dynamic-versioning setup aligns with PR objectives.
5-30: LGTM: Project metadata complete.Name, version strategy, Python range, and classifiers are appropriate.
44-64: LGTM: Tool configuration appropriate.Version sourcing, build targets, and package includes are properly configured.
sqlalchemy_redshift/__init__.py (2)
1-2: LGTM: Modern metadata imports.Proper use of
importlib.metadataandpackaging.versionreplaces legacypkg_resources.
12-12: LGTM: Package name matches pyproject.toml.Version retrieval uses correct package identifier.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pyproject.toml(1 hunks)sqlalchemy_redshift/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
sqlalchemy_redshift/__init__.py
9-9: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
sqlalchemy_redshift/__init__.py (3)
1-2: LGTM: Modern imports.Clean migration to
importlib.metadataandpackagingreplaces deprecatedpkg_resources.
14-14: LGTM: Package name matches pyproject.toml.Version retrieval uses correct package name
deepnote-sqlalchemy-redshift.
18-30: LGTM: Dialect registrations updated.Correctly registers three dialects, psycopg2cffi removed per PR objectives. Aligns with
pyproject.tomlentry points.
psycopg2cffias it is unmaintained and not used internallySummary by CodeRabbit
Chores
Documentation
Tests